Replace adm-zip with yauzl #35
Conversation
I don't think we can do that; the clangd binary requires the other files in the zip file (dynamically linked library files, and in most configurations built-in header files) to operate correctly. Regarding yauzl as a choice of library to use: it seems like it has a lower-level API which is more involved to use. What do you think about going back to unzipper? We switched away from it in #25 due to a bug in it, but it looks like that bug has since been resolved. Switching back to it could potentially be as simple as reverting #25. also cc @fannheyward for any thoughts |
|
unzipper's issues and pull request count don't inspire confidence :( https://github.com/ZJONSSON/node-unzipper#readme I can change the implementation to extract all files, that's straightforward. |
tamird
force-pushed
the
replace-adm-zip
branch
from
b9ced16 to
6e652ba
Compare
|
@HighCommander4 I've updated this to extract the entire archive, as before. |
All adm-zip versions after 0.5.10 seem to have bugs: - 0.5.11 causes tests to fail. - 0.5.12 causes tests to hang. - 0.5.13 causes extracted files to be empty.
tamird
force-pushed
the
replace-adm-zip
branch
from
6e652ba to
00e8d4a
Compare
|
I'm hesitant about replacing a use of a library that was basically a single function call, with one that involves this much more complexity, with four levels of nested callbacks and such; it seems like this would require more effort to maintain going forward. I see that #34 has been updated with a change that makes the test pass -- any reason not to go forward with that? (Also added @hokein for a second opinion.) |
|
The change that was made in #34 makes me even more suspicious. It seems to me that the maintainers of adm-zip are a bit sloppy, and that's enough to make me think we should look for an alternative, and yauzl is the most popular library I could find that also has a reasonable support record. Having said that, downloading clangd is a relatively small part of this library's functionality, so I don't feel very strongly about this. If we go with #34 we should document the reason for avoiding upgrading adm-zip, particularly since our tests fail to detect the 0.5.13+ issue. |
|
I'm OK with replacing adm-zip with another library, just a bit difficult to understand yauzl's API |
All adm-zip versions after 0.5.10 seem to have bugs:
This also avoids extracting anything other than the clangd binary.
This is an alternative to #34.